[OrderedDictionary] forward ordered dictionary values equality to values property #335

vanvoorden · 2023-12-08T00:59:09Z

Background

The current implementation of OrderedDictionary.Values.== forwards to Sequence.elementsEqual, which needs to perform a linear-time (O[N]) comparison over both collections.[1]

Since the "backing store" of our OrderedDictionary.Values is a ContiguousArray, we can forward that equality check through to ContiguousArray. If both Values instances point to the same identity, the ContiguousArray implementation will return early from the comparison.[2]

We perform a similar check (against the ContiguousArray) in our implementation of OrderedDictionary.==.[3]

[1] https://github.com/apple/swift/blob/main/stdlib/public/core/SequenceAlgorithms.swift#L319-L336
[2] https://github.com/apple/swift/blob/main/stdlib/public/core/ContiguousArray.swift#L1318-L1321
[3] https://github.com/apple/swift-collections/blob/main/Sources/OrderedCollections/OrderedDictionary/OrderedDictionary%2BEquatable.swift#L20-L22

Changes

We migrate from:

public static func ==(left: Self, right: Self) -> Bool {
  left.elementsEqual(right)
}

We migrate to:

public static func ==(left: Self, right: Self) -> Bool {
  left._base._values == right._base._values
}

Test Plan

Two new tests are added:

OrderedDictionaryValueTests.test_values_getter_equal
OrderedDictionaryValueTests.test_values_getter_not_equal

Checklist

I've read the Contribution Guidelines
My contributions are licensed under the Swift license.
I've followed the coding style of the rest of the project.
I've added tests covering all new code paths my change adds to the project (if appropriate).
I've added benchmarks covering new functionality (if appropriate).
I've verified that my change does not break any existing tests or introduce unexplained benchmark regressions.
I've updated the documentation if necessary.

vanvoorden · 2023-12-08T01:18:51Z

@swift-ci test

vanvoorden · 2023-12-08T01:44:31Z

@ingoem thanks for suggesting the reference equality check!

lorentey

Nice! 👍

Tests/_CollectionsTestSupport/AssertionContexts/Assertions.swift

lorentey · 2023-12-08T22:31:44Z

@vanvoorden Can you please rebase this PR on top of the release/1.0 branch, rather than main?

I think it would make sense to ship this in the next patch release, rather than having it linger on main indefinitely.

vanvoorden · 2023-12-09T04:41:46Z

I think it would make sense to ship this in the next patch release, rather than having it linger on main indefinitely.

@lorentey Sounds good. Thanks!

vanvoorden · 2023-12-11T20:21:06Z

@lorentey Is there a CI job that can run Benchmarks on a diff to detect for regressions or improvements against base?

vanvoorden · 2023-12-12T00:00:27Z

Here are benchmarks running locally. The reason that OrderedDictionary.== is scaling linearly is because we are waiting on #340. Optimizing the OrderedSet equality will optimize this one.

lorentey

Thanks for rebasing. Nice work on the benchmarks! 👍

Reviewing #340 made me realize what's been bothering me about expectNotEqualElements; I added new recommendations to remove it and the test that exercises it. (Please see the notes below.)

Tests/OrderedCollectionsTests/OrderedDictionary/OrderedDictionary+Values Tests.swift

Sources/_CollectionsTestSupport/AssertionContexts/Assertions.swift

Tests/OrderedCollectionsTests/OrderedDictionary/OrderedDictionary+Values Tests.swift

Co-authored-by: Karoy Lorentey <klorentey@apple.com>

vanvoorden · 2023-12-12T19:19:18Z

@lorentey Thanks for reviewing!

lorentey · 2023-12-12T19:29:36Z

@swift-ci test

lorentey · 2023-12-12T19:30:59Z

(AFAIK, CI runs are still expected to fail on macOS. This is "fine", and it won't block us from landing this.)

…v1.0.6 (#822) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [com_github_apple_swift_collections](https://togithub.com/apple/swift-collections) | http_archive | patch | `1.0.5` -> `1.0.6` | --- ### Release Notes <details> <summary>apple/swift-collections (com_github_apple_swift_collections)</summary> ### [`v1.0.6`](https://togithub.com/apple/swift-collections/releases/tag/1.0.6): Swift Collections 1.0.6 [Compare Source](https://togithub.com/apple/swift-collections/compare/1.0.5...1.0.6) This bugfix release adds `Sendable` conformances to all public types (fixing compatibility with Swift's strict concurrency checking), and speeds up equality checks (`==`) of identical collection values. ##### What's Changed - Fix typos: OrderedSet Documentation by [@kati-kms](https://togithub.com/kati-kms) in [https://github.com/apple/swift-collections/pull/322](https://togithub.com/apple/swift-collections/pull/322) - \[1.0] build: support building in Debug mode on Windows by [@compnerd](https://togithub.com/compnerd) in [https://github.com/apple/swift-collections/pull/337](https://togithub.com/apple/swift-collections/pull/337) - build: tweak search path for embedding by [@compnerd](https://togithub.com/compnerd) in [https://github.com/apple/swift-collections/pull/338](https://togithub.com/apple/swift-collections/pull/338) - \[OrderedDictionary] forward ordered dictionary values equality to values property by [@vanvoorden](https://togithub.com/vanvoorden) in [https://github.com/apple/swift-collections/pull/335](https://togithub.com/apple/swift-collections/pull/335) - \[OrderedSet] forward ordered set equality to elements property by [@vanvoorden](https://togithub.com/vanvoorden) in [https://github.com/apple/swift-collections/pull/340](https://togithub.com/apple/swift-collections/pull/340) - \[Deque] check deque equality with buffer identity by [@vanvoorden](https://togithub.com/vanvoorden) in [https://github.com/apple/swift-collections/pull/341](https://togithub.com/apple/swift-collections/pull/341) - \[OrderedDictionary] Fix usage of deprecated API in index(forKey:) docs by [@lorentey](https://togithub.com/lorentey) in [https://github.com/apple/swift-collections/pull/342](https://togithub.com/apple/swift-collections/pull/342) - \[1.0] Backport Sendable conformances on all public types by [@lorentey](https://togithub.com/lorentey) in [https://github.com/apple/swift-collections/pull/343](https://togithub.com/apple/swift-collections/pull/343) - OrderedSet: Fix sendable conformance on old swifts by [@lorentey](https://togithub.com/lorentey) in [https://github.com/apple/swift-collections/pull/346](https://togithub.com/apple/swift-collections/pull/346) - Update CMake configuration by [@lorentey](https://togithub.com/lorentey) in [https://github.com/apple/swift-collections/pull/347](https://togithub.com/apple/swift-collections/pull/347) ##### New Contributors - [@kati-kms](https://togithub.com/kati-kms) made their first contribution in [https://github.com/apple/swift-collections/pull/322](https://togithub.com/apple/swift-collections/pull/322) - [@vanvoorden](https://togithub.com/vanvoorden) made their first contribution in [https://github.com/apple/swift-collections/pull/335](https://togithub.com/apple/swift-collections/pull/335) **Full Changelog**: apple/swift-collections@1.0.5...1.0.6 Thank you to everyone who contributed to this release! </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://togithub.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate).  Co-authored-by: Self-hosted Renovate Bot <361546+cgrindel-self-hosted-renovate[bot]@users.noreply.github.enterprise.com>

vanvoorden requested a review from lorentey as a code owner December 8, 2023 00:59

lorentey approved these changes Dec 8, 2023

View reviewed changes

Tests/_CollectionsTestSupport/AssertionContexts/Assertions.swift Outdated Show resolved Hide resolved

lorentey added this to the 1.0.6 milestone Dec 8, 2023

vanvoorden changed the base branch from main to release/1.0 December 9, 2023 01:12

vanvoorden changed the base branch from release/1.0 to main December 9, 2023 01:12

vanvoorden changed the base branch from main to release/1.0 December 9, 2023 01:30

vanvoorden changed the base branch from release/1.0 to main December 9, 2023 01:30

forward ordered dictionary values equality to values property

e42fb86

vanvoorden force-pushed the vanvoorden/ordered-dictionary-values-equality branch from 598b5f9 to e42fb86 Compare December 9, 2023 01:42

vanvoorden changed the base branch from main to release/1.0 December 9, 2023 01:42

vanvoorden mentioned this pull request Dec 9, 2023

[OrderedSet] forward ordered set equality to elements property #340

Merged

7 tasks

ordered dictionary equality benchmarks

a71f2cc

vanvoorden mentioned this pull request Dec 12, 2023

[Deque] check deque equality with buffer identity #341

Merged

7 tasks

lorentey added the OrderedCollections OrderedSet and OrderedDictionary label Dec 12, 2023

lorentey approved these changes Dec 12, 2023

View reviewed changes

vanvoorden and others added 3 commits December 12, 2023 11:17

add ordered dictionary test for equal identity

baa3880

Co-authored-by: Karoy Lorentey <klorentey@apple.com>

remove extra test equality helper function

5fa8e44

Co-authored-by: Karoy Lorentey <klorentey@apple.com>

remove extra ordered dictionary elements test

d266289

Co-authored-by: Karoy Lorentey <klorentey@apple.com>

lorentey merged commit 2a94309 into apple:release/1.0 Dec 12, 2023
1 of 2 checks passed

vanvoorden mentioned this pull request Dec 12, 2023

OrderedDictionary.values and OrderedSet Performance Problems when SwiftUI.List is paired with SwiftUI.NavigationStack #334

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OrderedDictionary] forward ordered dictionary values equality to values property #335

[OrderedDictionary] forward ordered dictionary values equality to values property #335

vanvoorden commented Dec 8, 2023 •

edited

Loading

vanvoorden commented Dec 8, 2023

vanvoorden commented Dec 8, 2023

lorentey left a comment

lorentey commented Dec 8, 2023

vanvoorden commented Dec 9, 2023

vanvoorden commented Dec 11, 2023

vanvoorden commented Dec 12, 2023

lorentey left a comment

vanvoorden commented Dec 12, 2023

lorentey commented Dec 12, 2023

lorentey commented Dec 12, 2023

[OrderedDictionary] forward ordered dictionary values equality to values property #335

[OrderedDictionary] forward ordered dictionary values equality to values property #335

Conversation

vanvoorden commented Dec 8, 2023 • edited Loading

Background

Changes

Test Plan

Checklist

vanvoorden commented Dec 8, 2023

vanvoorden commented Dec 8, 2023

lorentey left a comment

Choose a reason for hiding this comment

lorentey commented Dec 8, 2023

vanvoorden commented Dec 9, 2023

vanvoorden commented Dec 11, 2023

vanvoorden commented Dec 12, 2023

lorentey left a comment

Choose a reason for hiding this comment

vanvoorden commented Dec 12, 2023

lorentey commented Dec 12, 2023

lorentey commented Dec 12, 2023

vanvoorden commented Dec 8, 2023 •

edited

Loading